Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add methods to configure context lines for fingerprinting #1943

Open
wants to merge 5 commits into
base: 13.0.0
Choose a base branch
from
Open

Add methods to configure context lines for fingerprinting #1943

wants to merge 5 commits into from

Conversation

Jay-oao
Copy link

@Jay-oao Jay-oao commented Feb 3, 2025 •
edited
Loading

This PR introduces a tool-based configuration for setting the context lines considered during the fingerprinting process in FullTextFingerprint.
See :
https://issues.jenkins.io/browse/JENKINS-61607
jenkinsci/analysis-model#1136
jenkinsci/analysis-model-api-plugin#184

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

uhafner
uhafner requested changes Feb 3, 2025
View reviewed changes
Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UI elements are missing:

(There is also a test missing but this is optional here as this requires more work than the actual feature.)

plugin/src/main/java/io/jenkins/plugins/analysis/core/steps/IssuesScanner.java Outdated
@@ -365,7 +372,13 @@ private void createFingerprints(final Report report) {
report.logInfo("Creating fingerprints for all affected code blocks to track issues over different builds");

FingerprintGenerator generator = new FingerprintGenerator();
generator.run(new FullTextFingerprint(), report, getCharset());
if (linesLookAhead < 0) {
//Empty constructor defaults context lines to 3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//Empty constructor defaults context lines to 3
// no-arg constructor defaults context lines to 3

@uhafner
Copy link
Member

uhafner commented Feb 3, 2025

The broken tests are not you fault: somehow the Java 17 Jenkins release does not work with my docker tests right now. I am still trying to understand what is going on here...

@Jay-oao
Copy link
Author

Jay-oao commented Feb 5, 2025 •
edited
Loading

The UI elements are missing:

(There is also a test missing but this is optional here as this requires more work than the actual feature.)

I'd be happy to help if you could guide me on the steps to get started with the tests.

@uhafner
Copy link
Member

uhafner commented Feb 5, 2025

(There is also a test missing but this is optional here as this requires more work than the actual feature.)

I'd be happy to help if you could guide me on the steps to get started with the tests.

For a test a good staring point are the test cases in AffectedFilesResolverITest: these tests verify that the affected files are read and processed. Maybe we can create a new test that extends this functionality?

Currently theses test have one build to show a warning in a file. What we need is: two builds where the warning is moved in the file by e.g. one line in the second build (so we need the same source code file slightly modified by inserting a new line somewhere before). This warning should be marked as existing warning based on a small value of the fingerprint. But when the changed line is included in the fingerprint then the warning should still be marked as existing.

@Jay-oao
Copy link
Author

Jay-oao commented Feb 8, 2025

For a test a good staring point are the test cases in AffectedFilesResolverITest: these tests verify that the affected files are read and processed. Maybe we can create a new test that extends this functionality?

Currently theses test have one build to show a warning in a file. What we need is: two builds where the warning is moved in the file by e.g. one line in the second build (so we need the same source code file slightly modified by inserting a new line somewhere before). This warning should be marked as existing warning based on a small value of the fingerprint. But when the changed line is included in the fingerprint then the warning should still be marked as existing.

When the changed line is included in the fingerprinting of the second build , wont the fingerprint change and result in a new issue ? Or am I missing something else here ?

@uhafner
Copy link
Member

uhafner commented Feb 9, 2025

For a test a good staring point are the test cases in AffectedFilesResolverITest: these tests verify that the affected files are read and processed. Maybe we can create a new test that extends this functionality?
Currently theses test have one build to show a warning in a file. What we need is: two builds where the warning is moved in the file by e.g. one line in the second build (so we need the same source code file slightly modified by inserting a new line somewhere before). This warning should be marked as existing warning based on a small value of the fingerprint. But when the changed line is included in the fingerprint then the warning should still be marked as existing.

When the changed line is included in the fingerprinting of the second build , wont the fingerprint change and result in a new issue ? Or am I missing something else here ?

This is exactly the point. The fingerprint is changed with the default setting, but not with the changed setting. So the build should have a different result depending on the new property: then we prove that the new setting works.

@uhafner
Copy link
Member

uhafner commented Feb 9, 2025

BTW: I disabled the broken tests in the 13.x branch, maybe it makes sense to rebase so that your build is green as well...

@Jay-oao
Copy link
Author

Jay-oao commented Feb 10, 2025

For a test a good staring point are the test cases in AffectedFilesResolverITest: these tests verify that the affected files are read and processed. Maybe we can create a new test that extends this functionality?
Currently theses test have one build to show a warning in a file. What we need is: two builds where the warning is moved in the file by e.g. one line in the second build (so we need the same source code file slightly modified by inserting a new line somewhere before). This warning should be marked as existing warning based on a small value of the fingerprint. But when the changed line is included in the fingerprint then the warning should still be marked as existing.

When the changed line is included in the fingerprinting of the second build , wont the fingerprint change and result in a new issue ? Or am I missing something else here ?

This is exactly the point. The fingerprint is changed with the default setting, but not with the changed setting. So the build should have a different result depending on the new property: then we prove that the new setting works.

oh ok i think i get it now , i will push changes in a bit . Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uhafner uhafner uhafner approved these changes

Assignees

@uhafner uhafner

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Jay-oao @uhafner